Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughdocumentation pr adds three maintainer runbooks for safe workflows (adding Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes notes (senior staff engineer tone, lowercase): flag missing regression tests for behavioral changes referenced by the runbooks. verify referenced code locations ( 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/development/RUNBOOK_ADD_AUTH_MANAGER_COMMAND.md`:
- Around line 9-15: Docs omission: add a note that interactive/prompt-heavy
commands also use lib/cli.ts and its prompt helpers; update the "Where to
Change" list to include lib/cli.ts and mention the specific prompt helpers
(promptAddAnotherAccount, promptLoginMode) referenced by lib/codex-manager.ts so
maintainers know to check lib/cli.ts when adding interactive commands.
In `@docs/development/RUNBOOK_ADD_CONFIG_FIELD_SAFELY.md`:
- Around line 9-18: Add lib/schemas.ts to the touchpoint list and note that new
config fields must update PluginConfigSchema (refer to PluginConfigSchema in
lib/schemas.ts) in addition to lib/config.ts; also add a line referencing the
Windows filesystem EBUSY/EPERM retry guidance from
docs/development/CONFIG_FIELDS.md so readers know to follow the documented retry
behavior for Windows edge cases when modifying persisted config.
In `@docs/development/RUNBOOK_CHANGE_ROUTING_POLICY_SAFELY.md`:
- Around line 9-17: The runbook omitted lib/accounts.ts from the list of
touchpoints; add lib/accounts.ts to the "Where to Change" section because
routing and account-selection logic reads/filters/scores accounts there. Update
the list alongside index.ts, lib/rotation.ts, etc., and mention that policy
changes affecting account data formats or scoring should update functions/types
in lib/accounts.ts (e.g., the account parsing/filtering and scoring helpers) so
reviewers know to check that file.
In `@test/documentation.test.ts`:
- Around line 270-284: The runbook existence assertions (the runbooks array and
the expects checking join(projectRoot, fieldInventoryPath) and join(projectRoot,
runbook)) are placed inside the wrong test scope; extract them into their own
test case named something like "has all required maintainer runbooks" instead of
leaving them inside "documents stable overrides separately from advanced and
internal overrides". Create a new test block that iterates the runbooks array
and asserts existsSync(join(projectRoot, ...)) for each entry (and the
fieldInventoryPath) and remove those assertions from the original test so each
test focuses on a single concern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c56c3537-3e83-4fd3-b9c7-668836311bcf
📒 Files selected for processing (6)
docs/README.mddocs/development/RUNBOOK_ADD_AUTH_MANAGER_COMMAND.mddocs/development/RUNBOOK_ADD_CONFIG_FIELD_SAFELY.mddocs/development/RUNBOOK_CHANGE_ROUTING_POLICY_SAFELY.mddocs/releases/v1.2.0.mdtest/documentation.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/documentation.test.ts
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/README.mddocs/development/RUNBOOK_ADD_CONFIG_FIELD_SAFELY.mddocs/releases/v1.2.0.mddocs/development/RUNBOOK_CHANGE_ROUTING_POLICY_SAFELY.mddocs/development/RUNBOOK_ADD_AUTH_MANAGER_COMMAND.md
🔇 Additional comments (2)
docs/releases/v1.2.0.md (1)
1-18: lgtm — release stub is consistent with package.json.the version
1.2.0and package namecodex-multi-authmatchpackage.json:2-3. relative links in the related section will be validated by the internal link test attest/documentation.test.ts:529-551.docs/README.md (1)
66-68: lgtm — runbook links added to maintainer docs index.the three new entries point to the runbook files added in this pr.
test/documentation.test.ts:529-551will validate these links resolve.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/development/RUNBOOK_ADD_AUTH_MANAGER_COMMAND.md`:
- Around line 32-37: Update the "QA" section in the docs/dev runbook to add an
explicit token-safety callout before the "Run the real command or `--help` path"
step: instruct users to redact or avoid sharing any tokens/session data (API
keys, access tokens, session IDs) when capturing logs or output, and reference
the existing safety check pattern in test/documentation.test.ts (lines ~457-461)
so the wording aligns with project security guidelines; place this new callout
immediately above the real-command inspection bullet and ensure it clearly tells
QA to sanitize outputs before sharing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6dc4e48c-20ce-4339-abff-19723ce6c067
📒 Files selected for processing (4)
docs/development/RUNBOOK_ADD_AUTH_MANAGER_COMMAND.mddocs/development/RUNBOOK_ADD_CONFIG_FIELD_SAFELY.mddocs/development/RUNBOOK_CHANGE_ROUTING_POLICY_SAFELY.mdtest/documentation.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/documentation.test.ts
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/development/RUNBOOK_ADD_CONFIG_FIELD_SAFELY.mddocs/development/RUNBOOK_ADD_AUTH_MANAGER_COMMAND.mddocs/development/RUNBOOK_CHANGE_ROUTING_POLICY_SAFELY.md
🔇 Additional comments (3)
test/documentation.test.ts (1)
266-279: good extraction into a dedicated runbook-presence test.line 266 keeps this concern isolated and deterministic, and it aligns with the surrounding docs integrity suite in
test/documentation.test.ts:104-556.docs/development/RUNBOOK_ADD_CONFIG_FIELD_SAFELY.md (1)
11-41: looks solid. schemas + windows persistence guidance are now explicit.the touchpoints and compatibility checks are clear, and line 34 now anchors the
ebusy/epermexpectation that aligns with docs integrity coverage intest/documentation.test.ts:296-301.docs/development/RUNBOOK_CHANGE_ROUTING_POLICY_SAFELY.md (1)
11-38: good coverage of routing-policy touchpoints and validation flow.including
lib/accounts.tson line 12 and the focused policy test set on line 18 makes this runbook actionable and consistent with related checks intest/documentation.test.ts:383-420.
Summary
Validation
npm run test -- test/documentation.test.ts(fails due to pre-existing broken links todocs/releases/v1.2.0.md, unrelated to this PR)note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
adds three maintainer runbooks (auth/manager command, config field, routing policy), links them from the docs portal, creates the stub
docs/releases/v1.2.0.mdto fix a pre-existing broken link, and extends the documentation integrity test to assert all three runbooks are present.RUNBOOK_ADD_CONFIG_FIELD_SAFELY.mdcompatibility check mentionsEBUSYandEPERMbut omitsEAGAIN, which is part of the documented retry contract inAGENTS.mdkeeps maintainer runbooks presenttest usesexistsSynconly — a truncated or empty runbook would still pass; other docs tests in this file validate section headings and this one could toodocs/releases/v1.2.0.mdis a minimal anchor stub; it resolves the broken-link pre-condition the PR description calls out, though it contains no actual changelog contentConfidence Score: 5/5
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[maintainer wants to change codebase] --> B{what type of change?} B -->|new codex auth command| C[RUNBOOK_ADD_AUTH_MANAGER_COMMAND.md] B -->|new config/settings field| D[RUNBOOK_ADD_CONFIG_FIELD_SAFELY.md] B -->|routing/account-selection policy| E[RUNBOOK_CHANGE_ROUTING_POLICY_SAFELY.md] C --> C1[lib/codex-manager.ts + lib/codex-manager/] C --> C2[docs/reference/commands.md] C --> C3[test/codex-manager-cli.test.ts + test/documentation.test.ts] C --> C4[npm run typecheck + lint + test] D --> D1[lib/config.ts + lib/schemas.ts] D --> D2[lib/dashboard-settings.ts / lib/unified-settings.ts] D --> D3[docs/configuration.md + CONFIG_FIELDS.md] D --> D4[test/config.test.ts + test/unified-settings.test.ts] D --> D5[Windows: EBUSY/EPERM/EAGAIN retry must remain covered] E --> E1[lib/rotation.ts + lib/forecast.ts] E --> E2[lib/request/failure-policy.ts + rate-limit-backoff.ts] E --> E3[test/rotation.test.ts + test/forecast.test.ts + test/failure-policy.test.ts] E --> E4[npm run typecheck + lint + targeted policy tests]Prompt To Fix All With AI
Last reviewed commit: "Add auth log redacti..."